Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @koorosh)

pkg/server/serverpb/status.proto, line 1721 at r1 (raw file):

  }

  rpc HotRanges2(HotRangesRequest) returns (HotRangesResponseV2) {

Can we reconsider the placement of this API call? We already have an /api/v2/ server that new stuff should go under and I'd prefer to use that if we can.

See https://github.com/cockroachdb/cockroach/blob/master/pkg/server/api_v2.go

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)

pkg/server/serverpb/status.proto, line 1721 at r1 (raw file):

Done. I updated /api/v2/ranges/hot api to provide updated version of Hot ranges, but I would prefer to keep /_status/v2/hotranges as well until /api/v2/ranges/hot handles authorization validation (for secure and insecure modes).

Nice work! It's great to see this coming together.

One general question I wanted to pose here is have we considered how this might work in a serverless context? With the upcoming project to improve tenant-level observability, we'd ideally like to be able to generate a hot ranges report for a specific serverless tenant. We're hoping to build some level of debug.zip functionality at the tenant level, and one of the pieces that we think will be useful for such a bundle is a hot ranges report, since this is relevant within the SQL domain.

This is not necessarily in scope for this PR, but we should think about it now instead of later to avoid having to build a HotRangesV3 endpoint a few months from now 😅. What would we need to do to make this work at the tenant-level?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, and @koorosh)

pkg/server/status.go, line 2073 at r4 (raw file):

	}

	dbNames := make(map[uint32]string)

Can we possibly make a struct type to more clearly keep track of all this information?

It seems like the key for all these maps is the catalog.Descriptor ID, so instead of having multiple maps, can we have a single struct type that encompasses all these attributes (db names, table names, index names, and parent ID), and then just use a single map? Something like

type HotRangeReportMeta struct {
    dbName string
    tableName string
    indexNames map[uint32]string
    parentId int32
}
rangeReportMetas := make(map[uint32]HotRangeReportMeta)
...
for _, desc := range descrs {
    rangeReportMetas[uint32(desc.GetId())] = HotRangeReportMeta{
        ...
    }
} 

Then the maintainer can get all the info they need with a single map access. Just an idea, let me know what you think. We could also potentially define functions on this type to encapsulate some of the more complex access patterns of this data, although maybe that's overkill :D

Code quote:

	dbNames := make(map[uint32]string)
	tableNames := make(map[uint32]string)
	indexNames := make(map[uint32]map[uint32]string)
	parents := make(map[uint32]uint32)

pkg/server/status.go, line 2106 at r4 (raw file):

	var ranges []*serverpb.HotRangesResponseV2_HotRange
	// TODO (koorosh): how to flatten triple nested loop?

Nothing stands out to me that would allow us to avoid this, I don't see it as too big of an issue, personally :)

Code quote:

// TODO (koorosh): how to flatten triple nested loop?

Reviewed 1 of 6 files at r1, 8 of 8 files at r4, 5 of 5 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @koorosh)

-- commits, line 8 at r4:
nit: can you reword this a bit? It seems there are two changes being made with v2: not exposing sensitive data and creating a flat structure. Please make that a bit more clear.

-- commits, line 16 at r5:
nit: use backticks for fields

-- commits, line 21 at r5:
nit: can you add a note about why this would lead to additional store scans?

-- commits, line 28 at r6:
nit: can you clarify what the implementation was before and after the change. It's unclear from this sentence.

docs/generated/swagger/spec.json, line 1779 at r6 (raw file):

    },
    "hotRangeInfo": {
      "description": "Hot range details",

This should be more detailed.

pkg/server/status.go, line 2065 at r4 (raw file):

}

func (s *statusServer) HotRanges2(

nit: docstring

pkg/server/serverpb/status.proto, line 1721 at r1 (raw file):

SGTM

pkg/server/serverpb/status.proto, line 1126 at r5 (raw file):

message HotRangesResponseV2 {
  message HotRange {

nit: docstring

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)

-- commits, line 8 at r4:

Done.

-- commits, line 16 at r5:

Done.

-- commits, line 21 at r5:

Done.

-- commits, line 28 at r6:

Done.

docs/generated/swagger/spec.json, line 1779 at r6 (raw file):

Done.

pkg/server/status.go, line 2065 at r4 (raw file):

Done.

pkg/server/status.go, line 2073 at r4 (raw file):

Done.

pkg/server/status.go, line 2106 at r4 (raw file):

Ok.

One general question I wanted to pose here is have we considered how this might work in a serverless context?

Just noting - we (myself, @koorosh, @rimadeodhar) had a chat about this today. The effort involved to make this work in a serverless context will begin with our work to get a minimal version of debug.zip working for serverless tenants. Since debug.zip uses the original HotRanges endpoint, which is also used by this new version, the work we do on the original endpoint should carry over capabilities to the new version as well.

The effort involved should be somewhat minimal - we just need to filter down the ranges/replicas reported on to only those with the appropriate tenant key prefix.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)

Nice work, I think this is almost ready. I just have a few docstring nits and an error condition that I think we should probably log.

There are also some test failures in the CI build - have you tried running make testrace or make stressrace locally? I've re-triggered them in case it was a spurious failure, but if not, let me know if I can help debug 👍

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, and @koorosh)

pkg/server/status.go, line 2101 at r26 (raw file):

				_, tableID, err := s.sqlServer.execCfg.Codec.DecodeTablePrefix(r.Desc.StartKey.AsRawKey())
				if err != nil {
					continue

nit: should we consider at least logging the error here?

Code quote:

					continue

pkg/server/serverpb/status.proto, line 1071 at r26 (raw file):

    // API: PUBLIC ALPHA
    double queries_per_second = 2;
    // LeaseholderNodeID indicates on Node ID that contains replica that is leaseholder

nit: // LeaseholderNodeID indicates the Node ID that is the current leaseholder for the given range.

Code quote:

// LeaseholderNodeID indicates on Node ID that contains replica that is leaseholder

pkg/server/serverpb/status.proto, line 1129 at r26 (raw file):

  // HotRange message describes a single hot range, ie its QPS, node ID it belongs to, etc.
  message HotRange {
    // range_id indicates Range ID that's identified as hot range

nit: for this docstring and those below, let's make sure the sentences end with . periods to stay consistent with the rest of the file

pkg/server/serverpb/status.proto, line 1134 at r26 (raw file):

      (gogoproto.customname) = "RangeID"
    ];
    // node_id indicates on node that contains current hot range

nit: // node_id indicates the node that contains the current hot range.

Code quote:

// node_id indicates on node that contains current hot range

pkg/server/serverpb/status.proto, line 1144 at r26 (raw file):

      (gogoproto.customname) = "QPS"
    ];
    // table_name indicates table which data is stored in this hot range

nit: // table_name indicates the SQL table that the range belongs to.

Code quote:

// table_name indicates table which data is stored in this hot range

pkg/server/serverpb/status.proto, line 1155 at r26 (raw file):

        "github.com/cockroachdb/cockroach/pkg/roachpb.NodeID"
    ];
    // leaseholder_node_id indicates on Node ID that contains replica that is a leaseholder

nit: // leaseholder_node_id indicates the Node ID that is the current leaseholder for the given range.

Code quote:

/ leaseholder_node_id indicates on Node ID that contains replica that is a leaseholder

Thanks for pointing out on this, I didn't run them. Will run locally and to check what's wrong

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)

pkg/server/status.go, line 2101 at r26 (raw file):

Done.

pkg/server/serverpb/status.proto, line 1071 at r26 (raw file):

Done.

pkg/server/serverpb/status.proto, line 1129 at r26 (raw file):

Done.

pkg/server/serverpb/status.proto, line 1134 at r26 (raw file):

Done.

pkg/server/serverpb/status.proto, line 1144 at r26 (raw file):

Done.

pkg/server/serverpb/status.proto, line 1155 at r26 (raw file):

// leaseholder_node_id indicates the Node ID that is the current leaseholder for the given range.
Done.

Can we squash some of the commits in this PR? We shouldn't have any "nit" commits in history. I'd prefer if we just made this PR a single commit but I can understand if you want to break it up into 2 or 3. Right now it's too many.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)